Flow.Launcher/Helper/AutoStartup.cs (1)

173-200: 🛠️ Refactor suggestion

Add error handling for administrator privilege settings.

The method doesn't explicitly check if setting the RunLevel was successful, which could lead to silent failures.

 private static bool ScheduleLogonTask(bool alwaysRunAsAdministrator)
 {
     using var td = TaskService.Instance.NewTask();
     td.RegistrationInfo.Description = LogonTaskDesc;
     td.Triggers.Add(new LogonTrigger { UserId = WindowsIdentity.GetCurrent().Name, Delay = TimeSpan.FromSeconds(2) });
     td.Actions.Add(Constant.ExecutablePath);

+    bool requestedElevation = false;
     // Only if the app is running as administrator, we can set the run level to highest
     if (Win32Helper.IsAdministrator() && alwaysRunAsAdministrator)
     {
         td.Principal.RunLevel = TaskRunLevel.Highest;
+        requestedElevation = true;
     }

     td.Settings.StopIfGoingOnBatteries = false;
     td.Settings.DisallowStartIfOnBatteries = false;
     td.Settings.ExecutionTimeLimit = TimeSpan.Zero;

     try
     {
-        TaskService.Instance.RootFolder.RegisterTaskDefinition(LogonTaskName, td);
+        var registeredTask = TaskService.Instance.RootFolder.RegisterTaskDefinition(LogonTaskName, td);
+        
+        // Verify that elevation was set if requested
+        if (requestedElevation && registeredTask.Definition.Principal.RunLevel != TaskRunLevel.Highest)
+        {
+            App.API.LogWarning(ClassName, "Failed to set task to run with highest privileges");
+            // We don't fail the operation, just log a warning
+        }
+        
         return true;
     }
     catch (Exception e)
     {
         App.API.LogError(ClassName, $"Failed to schedule logon task: {e}");
         return false;
     }
 }

Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)

56-66: Consider adding a warning about security implications.

Running applications with administrator privileges can have security implications. Consider adding a more detailed tooltip or warning about the potential risks of always running with elevated privileges.

Example enhancement:

<cc:Card
    Title="{DynamicResource alwaysRunAsAdministrator}"
    Icon="&#xE7EF;"
-   Sub="{DynamicResource alwaysRunAsAdministratorToolTip}">
+   Sub="{DynamicResource alwaysRunAsAdministratorToolTip}"
+   Style="{StaticResource WarningCardStyle}">
    <ui:ToggleSwitch
        IsOn="{Binding AlwaysRunAsAdministrator}"
        OffContent="{DynamicResource disable}"
        OnContent="{DynamicResource enable}" />
</cc:Card>

Note: This assumes a WarningCardStyle exists or would need to be created.

Plugins/Flow.Launcher.Plugin.Program/UACDialog.xaml.cs (3)

12-13: Consider adding XML documentation and using consistent naming conventions for static fields.

Both msgBox and _result are static fields, but they use inconsistent naming conventions. Consider prefixing both with underscore for consistency, or use a consistent naming style.

-        private static UACDialog msgBox;
+        private static UACDialog _msgBox;
         private static MessageBoxResult _result = MessageBoxResult.None;

---

68-78: Clean up dialog instance consistently in button handlers.

The Button_Click method sets the dialog instance to null, but the field is static and might be accessed from other threads. Consider applying the same pattern to the KeyEsc_OnPress method.

     private void KeyEsc_OnPress(object sender, ExecutedRoutedEventArgs e)
     {
         DialogResult = false;
         Close();
+        _result = MessageBoxResult.None;
+        _msgBox = null;
     }

     private void Button_Click(object sender, RoutedEventArgs e)
     {
         if (sender == btnYes)
             _result = MessageBoxResult.Yes;
         else if (sender == btnNo)
             _result = MessageBoxResult.No;
         else
             _result = MessageBoxResult.None;
-        msgBox.Close();
-        msgBox = null;
+        _msgBox.Close();
+        _msgBox = null;
     }

---

80-85: Use consistent naming in Button_Cancel method.

The Button_Cancel method references msgBox directly, but should use the renamed field if you adopt the suggestion above.

     private void Button_Cancel(object sender, RoutedEventArgs e)
     {
         _result = MessageBoxResult.Cancel;
-        msgBox.Close();
-        msgBox = null;
+        _msgBox.Close();
+        _msgBox = null;
     }

Flow.Launcher/Helper/AutoStartup.cs (1)

120-131: Document startup method changes for administrator mode.

The code includes a good comment explaining why registry startup doesn't support administrator mode, but could benefit from additional documentation.

 public static void ChangeToViaLogonTask(bool alwaysRunAsAdministrator)
 {
     Disable(false);
     Enable(true, alwaysRunAsAdministrator);
 }

 public static void ChangeToViaRegistry()
 {
     Disable(true);
+    // Registry startup doesn't support running as administrator because it doesn't have an option
+    // to elevate privileges like the Task Scheduler does with RunLevel.Highest
     // We do not need to use alwaysRunAsAdministrator for registry, so we just set false here
     Enable(false, false);
 }

Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (5)

4-5: Namespace collision risk between WPF and WinForms types

Adding using System.Windows; to a file that is already importing System.Windows.Forms increases the likelihood of ambiguous type resolutions (MessageBox, Screen, DragEventArgs, …).
Although you currently reference the enumerations by their full namespace (MessageBoxButton, MessageBoxResult) the next developer who tries to call MessageBox.Show (or similar) will hit a compile-time ambiguity.

Two low-effort mitigations:

-using System.Windows;
+using Wpf = System.Windows;

or import only the specific WPF types you need:

using System.Windows.MessageBoxButton;
using System.Windows.MessageBoxResult;

Either option keeps the intent clear and shields you from hidden compilation errors down the road.

---

26-27: Static cache of elevation status can become stale after restart-in-place

_isAdministrator is captured once when the SettingsPaneGeneralViewModel type is first touched.
If (in a future enhancement) you decide to elevate the process without a full application restart, this flag will remain false, giving you inconsistent behaviour.

Safer alternative: turn it into a computed property or refresh it inside CheckAdminChangeAndAskForRestart() right before the comparison.

-private static readonly bool _isAdministrator = Win32Helper.IsAdministrator();
+private static bool IsAdministrator => Win32Helper.IsAdministrator();

Then replace usages accordingly.

---

72-78: Restart prompt executes even when startup-task creation fails

CheckAdminChangeAndAskForRestart() is invoked regardless of whether AutoStartup.ChangeToViaLogonTask threw.
In the failure path we show the user two dialogs: one for the error and then another asking for a restart—yet we know the scheduled task is still out of sync.

Consider short-circuiting when the startup change fails or basing the restart prompt on the success flag returned by the helper.

---

94-113: Duplicate logic – consider extracting a helper

The UseLogonTaskForStartup setter repeats almost the same try/catch & restart-check block found in StartFlowLauncherOnSystemStartup.
Extracting this into a private method (e.g. UpdateStartupMethod(bool viaLogonTask)) would keep the two setters terse and DRY.

---

141-154: Restart helper could lose original command-line context

App.API.RestartApp(AlwaysRunAsAdministrator ? "runas" : string.Empty); forwards only the elevation flag.
If the user had started Flow Launcher with extra CLI arguments (portable mode, debug flags, etc.) they will be dropped during restart.

Recommend overloading RestartApp to accept an argument builder or capture Environment.GetCommandLineArgs() and re-use them when invoking the new process.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Flow.Launcher/MainWindow.xaml.cs:598

• [nitpick] Variable name 'text' is too generic; consider renaming to 'notifyIconText' or similar to clarify its purpose.

var text = Win32Helper.IsAdministrator() ?

Flow.Launcher/PublicAPIInstance.cs:93

• Verify that UpdateManager.RestartApp supports an overload with arguments; if not, this call will fail at runtime or cause a breaking change in the public API.

UpdateManager.RestartApp(Constant.ApplicationFileName, arguments);

Flow.Launcher.Infrastructure/NativeMethods.txt (1)

96-108: Eliminate the duplicate P/Invoke declarations.

GetShellWindow, GetWindowThreadProcessId, and OpenProcess are already declared earlier in this manifest; redeclaring them here invites silent drift and signature skew. Please keep a single definition for each native API.

Flow.Launcher/App.xaml.cs (1)

62-67: Handle UAC cancellation to avoid locking users out.

When AlwaysRunAsAdministrator is true, a non-elevated launch hits RestartApp(true) inside the constructor. If the user cancels the UAC prompt, Process.Start throws Win32Exception(1223) and the app crashes; on relaunch the same guard triggers again, so the user can never get back in to toggle the setting off. Even if you catch the exception later, the unconditional return; keeps the constructor from completing, leaving the app uninitialized.

Wrap the restart attempt in a try/catch that treats ERROR_CANCELLED (1223) as “restart failed”, keep the constructor running in that case, and optionally clear AlwaysRunAsAdministrator so the user regains control.

Flow.Launcher/PublicAPIInstance.cs (1)

625-672: Critical: Ignoring boolean return value from RunAsDesktopUser.

The code calls Win32Helper.RunAsDesktopUser (line 634) but only checks if errorInfo is non-empty (line 647) to determine success. The boolean return value is ignored. If the method returns false but leaves errorInfo empty, the code will incorrectly return true (line 652), creating false positives. This was previously flagged but not fixed.

Apply this diff to check the boolean return value:

 // Use command executer to run the process as desktop user if running as admin
 if (Win32Helper.IsAdministrator())
 {
     var result = Win32Helper.RunAsDesktopUser(
         Constant.CommandExecutablePath,
         Environment.CurrentDirectory,
         $"-StartProcess " +
         $"-FileName {AddDoubleQuotes(fileName)} " +
         $"-WorkingDirectory {AddDoubleQuotes(workingDirectory)} " +
         $"-Arguments {AddDoubleQuotes(arguments)} " +
         $"-UseShellExecute {useShellExecute} " +
         $"-Verb {AddDoubleQuotes(verb)} " +
         $"-CreateNoWindow {createNoWindow}",
         false,
         true, // Do not show the command window
         out var errorInfo);
-    if (!string.IsNullOrEmpty(errorInfo))
+    
+    if (!result)
     {
         LogError(ClassName, $"Failed to start process {fileName} with arguments {arguments} under {workingDirectory}: {errorInfo}");
+        return false;
     }

     return result;
 }

Flow.Launcher.Infrastructure/Win32Helper.cs (2)

1141-1163: CreateProcessWithTokenW requires a mutable command-line buffer.

CreateProcessWithTokenW may modify the command-line string in place. Passing a pinned immutable string (line 1144) can cause undefined behavior or access violations. This was previously flagged but not addressed.

Allocate a mutable char buffer instead:

+// Allocate mutable buffer for command line
+char* cmdLineMutable = null;
+var cmdLineStr = $"- {cmdLine}";
+if (!string.IsNullOrEmpty(cmdLineStr))
+{
+    cmdLineMutable = (char*)Marshal.StringToHGlobalUni(cmdLineStr);
+}
+
 fixed (char* appPtr = app)
-// Because argv[0] is the module name, C programmers generally repeat the module name as the first token in the command line
-// So we add one more dash before the command line to make command line work correctly
-fixed (char* cmdLinePtr = $"- {cmdLine}")
 fixed (char* currentDirPtr = currentDir)
 {
+    try
+    {
         if (!PInvoke.CreateProcessWithToken(
             hPrimaryToken,
             // If you need to access content in HKEY_CURRENT_USER, please set loadProfile to true
             loadProfile ? CREATE_PROCESS_LOGON_FLAGS.LOGON_WITH_PROFILE : 0,
             appPtr,
-            cmdLinePtr,
+            cmdLineMutable,
             // If you do not want to create a window for console app, please set createNoWindow to true
             createNoWindow ? PROCESS_CREATION_FLAGS.CREATE_NO_WINDOW : 0,
             null,
             currentDirPtr,
             &si,
             &pi))
         {
             errorInfo = $"CreateProcessWithTokenW failed: {Marshal.GetLastWin32Error()}";
             goto cleanup;
         }
+    }
+    finally
+    {
+        if (cmdLineMutable != null) Marshal.FreeHGlobal((nint)cmdLineMutable);
+    }
 }

---

1048-1083: Verify privilege adjustment succeeded.

AdjustTokenPrivileges returns success even when no privilege is actually enabled. You must explicitly check if lastError == ERROR_NOT_ALL_ASSIGNED to confirm the privilege was granted. This was previously flagged but not addressed.

Apply this diff to add the explicit check:

 PInvoke.AdjustTokenPrivileges(hProcessToken, false, &tp, 0, null, null);
 var lastError = Marshal.GetLastWin32Error();
 hProcessToken.Dispose();

-if (lastError != 0)
+if (lastError == (int)WIN32_ERROR.ERROR_NOT_ALL_ASSIGNED)
+{
+    errorInfo = $"Failed to enable SE_INCREASE_QUOTA privilege: privilege not granted";
+    return false;
+}
+
+if (lastError != 0)
 {
     errorInfo = $"AdjustTokenPrivileges failed: {lastError}";
     return false;
 }

Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)

120-145: Critical: Missing OnPropertyChanged() breaks UI bindings.

The AlwaysRunAsAdministrator property setter does not call OnPropertyChanged(), so UI bindings will not update when this property is changed programmatically. This was previously flagged and you acknowledged it as "Good", but the fix is not implemented in the current code.

Additionally, CheckAdminChangeAndAskForRestart() is only called when both StartFlowLauncherOnSystemStartup and UseLogonTaskForStartup are true (line 129). A user who manually starts Flow Launcher and toggles this flag will not be prompted to restart, leaving them in the wrong elevation state.

Apply this diff to add OnPropertyChanged() and ensure restart prompts work regardless of startup configuration:

 public bool AlwaysRunAsAdministrator
 {
     get => Settings.AlwaysRunAsAdministrator;
     set
     {
         if (AlwaysRunAsAdministrator == value) return;

         Settings.AlwaysRunAsAdministrator = value;

         if (StartFlowLauncherOnSystemStartup && UseLogonTaskForStartup)
         {
             try
             {
                 AutoStartup.ChangeToViaLogonTask(value);
             }
             catch (Exception e)
             {
                 App.API.ShowMsg(App.API.GetTranslation("setAutoStartFailed"), e.Message);
             }
-
-            // If we have enabled logon task startup, we need to check if we need to restart the app
-            // even if we encounter an error while setting the startup method
-            CheckAdminChangeAndAskForRestart();
         }
+
+        // Always check if restart is needed when toggling admin mode
+        CheckAdminChangeAndAskForRestart();
+        OnPropertyChanged();
     }
 }

Flow.Launcher.Infrastructure/Http/Http.cs (1)

229-230: Past review concerns about exception logging remain unaddressed.

Removing the unused exception variable is fine, but previous reviewers flagged that exceptions should be logged rather than silently swallowed. Silent failures make debugging HTTP issues difficult and hide important diagnostic information.

Consider adding exception logging as previously suggested:

-            catch (System.Exception)
+            catch (System.Exception ex)
             {
+                Log.Exception(ClassName, $"Error occurred while fetching string from URL <{url}>", ex);
                 return string.Empty;
             }

Was there a specific reason for not adding logging when addressing the previous review comments?

Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)

331-340: Consider passing createNoWindow parameter for completeness.

The call to Context.API.StartProcess doesn't pass info.CreateNoWindow. While the current code doesn't explicitly set this property (so it defaults to false), passing it would make the implementation more complete and future-proof.

Apply this diff:

 private static Process StartProcess(ProcessStartInfo info)
 {
     Context.API.StartProcess(
         info.FileName,
         workingDirectory: info.WorkingDirectory,
         argumentList: info.ArgumentList,
         useShellExecute: info.UseShellExecute,
-        verb: info.Verb);
+        verb: info.Verb,
+        createNoWindow: info.CreateNoWindow);
     return null;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3573
File: Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs:491-493
Timestamp: 2025-06-18T13:55:09.190Z
Learning: When opening Windows settings (like indexing options), de-elevation is not needed since these operations cannot bring security risks, even when Flow Launcher is running as administrator.

Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.

Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.

Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.

Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3573
File: Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs:491-493
Timestamp: 2025-06-18T13:55:09.190Z
Learning: When opening Windows settings (like indexing options), de-elevation is not needed since these operations cannot bring security risks, even when Flow Launcher is running as administrator.

Flow.Launcher.sln (1)

292-303: Restore x64/x86 mappings for Flow.Launcher.Command
The new helper exe still maps every x64/x86 configuration back to Any CPU, so you never produce true platform-specific builds—same problem noted earlier. Please wire the mappings to their matching platforms (or drop the extra configurations entirely if the project is genuinely Any CPU).

-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x64.ActiveCfg = Debug|Any CPU
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x64.Build.0 = Debug|Any CPU
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x64.ActiveCfg = Debug|x64
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x64.Build.0 = Debug|x64
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x86.ActiveCfg = Debug|Any CPU
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x86.Build.0 = Debug|Any CPU
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x86.ActiveCfg = Debug|x86
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Debug|x86.Build.0 = Debug|x86
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x64.ActiveCfg = Release|Any CPU
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x64.Build.0 = Release|Any CPU
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x64.ActiveCfg = Release|x64
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x64.Build.0 = Release|x64
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x86.ActiveCfg = Release|Any CPU
-		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x86.Build.0 = Release|Any CPU
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x86.ActiveCfg = Release|x86
+		{A9976C5C-B73A-4D29-B654-EF1C0C4C9C8C}.Release|x86.Build.0 = Release|x86

Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)

54-64: Consider adding explicit margin for UI consistency.

The implementation correctly follows the existing patterns for SettingsCard structure, bindings, and localization. The placement after startup settings is logical.

However, most other SettingsCards in this file have explicit Margin attributes (e.g., Margin="0 4 0 0" for related items). Consider adding a margin to maintain consistent spacing:

-            <ui:SettingsCard Description="{DynamicResource alwaysRunAsAdministratorToolTip}" Header="{DynamicResource alwaysRunAsAdministrator}">
+            <ui:SettingsCard Margin="0 4 0 0" Description="{DynamicResource alwaysRunAsAdministratorToolTip}" Header="{DynamicResource alwaysRunAsAdministrator}">

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.

Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)

68-80: LGTM! Well-structured SettingsCard for admin mode.

The new SettingsCard follows the established patterns in the file with proper icon, localization bindings, and ToggleSwitch structure. The XAML syntax is correct and consistent with other settings cards.

Optional: Consider placement relative to startup settings.

The linked issue #2639 requested the option be placed "near" the startup setting. Currently, the admin card is positioned after "hideOnStartup" (lines 54-66), which places it several cards away from the "StartFlowLauncherOnSystemStartup" expander (lines 35-52). You might consider:

1. Placing it as a sub-item within the StartFlowLauncherOnSystemStartup expander (lines 44-51), or
2. Positioning it immediately after the expander (after line 52) with appropriate margin

However, the current placement may be intentional to group visibility/behavior settings together, so this is purely a UX consideration rather than a functional issue.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro